Add CommandBuffer abstraction with backend-specific downcasting by MarijnS95 · Pull Request #1033 · llvm/offload-test-suite

MarijnS95 · 2026-03-30T13:22:49Z

Command buffer creation and management was previously spread across each backend's executeProgram() with no shared interface, making it impossible to manage command buffers from backend-agnostic code. This introduces a CommandBuffer base class on Device so that higher-level code can create and pass around command buffers without knowing the backend. Per-object allocator/pool ownership also prepares for future async execution where multiple command buffers may be in-flight with independent lifetimes.

DX: DXCommandBuffer owns Allocator, CmdList, Fence, Event
VK: VKCommandBuffer owns CmdPool, CmdBuffer; each submission creates a new CommandBuffer for independent lifetime management
MTL: MTLCommandBuffer wraps MTL::CommandBuffer

Device::createCommandBuffer() returns Expected<unique_ptr<CommandBuffer>> with a default "not supported" implementation.

Test plan

Verify DX backend compiles and passes existing tests
Verify VK backend compiles and passes existing tests
Verify MTL backend compiles and passes existing tests

🤖 Generated with Claude Code

llvm-beanz · 2026-03-30T16:00:31Z

+    if (!CBOrErr)
+      return CBOrErr.takeError();
+    auto CBOwner = std::move(*CBOrErr);
+    State.CB = &CBOwner->as<DXCommandBuffer>();


Seems like it would be better if the Invocation state owned the command buffer rather than a temporary object in the function here. If we really should have two copies of it we should probably make it a shared_ptr rather than a unique_ptr.

I don't think using a shared_ptr for a command buffer is a good idea. The usage of shared_ptr in the rendering backend API is there for resources that can be accessed across multiple threads. However, command buffers should be access from one thread at a time.

I am open for either storing it as a unique_ptr in the InvocationState or just passing it as an argument each time.

Good points, the InvocationState also lives on the stack here and owns all members, except for the backend-specific-typed command buffer which is merely a pointer reference to another stack member (which becomes dangling when CBOwner is dropped from the stack before InvocationState).

I also vouch for keeping it uniquely owned, that way command buffers can only be passed by reference and not accidentally held and modified in multiple places (trying very hard to forget Rust's natural ownership semantics and page in C++'s nonsensical ones 🙃).

The only downside of that is needing the backend-specific pointer easily accessible. Either the field is stored twice, or this backend-specific executeProgram() call uses the specific XxxCommandBuffer::create() call to immediately have the right type until every internal field access is replaced with a generic abstraction.

Command buffer creation and management was previously spread across each backend's executeProgram() with no shared interface, making it impossible to manage command buffers from backend-agnostic code. This introduces a CommandBuffer base class on Device so that higher-level code can create and pass around command buffers without knowing the backend. Per-object allocator/pool ownership also prepares for future async execution where multiple command buffers may be in-flight with independent lifetimes. - DX: DXCommandBuffer owns Allocator, CmdList, Fence, Event - VK: VKCommandBuffer owns CmdPool, CmdBuffer; each submission creates a new CommandBuffer for independent lifetime management - MTL: MTLCommandBuffer wraps MTL::CommandBuffer Device::createCommandBuffer() returns Expected<unique_ptr<CommandBuffer>> with a default "not supported" implementation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

bogner

Is this just so that we can put command buffers from different backends in shared containers, or are you planning to follow up with adding backend agnostic APIs to this interface? Since all of the uses of these types just reach into the object and manipulate the backend specific objects type anyways it isn't really clear to me what this is for.

bogner · 2026-04-13T15:44:05Z

+//
+//
+//===----------------------------------------------------------------------===//


This bit of header comment isn't really necessary if we don't have any description of the module inside of it.

Ah, I saw it in the other PRs as well and thought that was introduced by clang-tidy as a default. Will remove it or add a description in the next PR.

bogner · 2026-04-13T15:50:46Z

+  template <typename T> T &as() {
+    assert(API == T::BackendAPI && "CommandBuffer backend mismatch");
+    return static_cast<T &>(*this);
+  }
+  template <typename T> const T &as() const {
+    assert(API == T::BackendAPI && "CommandBuffer backend mismatch");
+    return static_cast<const T &>(*this);
+  }


We should probably just set these classes up using LLVM-style RTTI rather than hand rolling it here: https://llvm.org/docs/HowToSetUpLLVMStyleRTTI.html

bogner · 2026-04-13T15:54:28Z

+  virtual llvm::Expected<std::unique_ptr<CommandBuffer>>
+  createCommandBuffer() = 0;


Too late to do anything about it, but note that the commit message claims:

Device::createCommandBuffer() returns Expected<unique_ptr> with a default "not supported" implementation.

However, this is a pure virtual function - there is no default implementation at all.

You're right - I fixed this last minute after realizing there were not actually any backends that leave this unimplemented, but forgot to update the commit message.

bogner · 2026-04-13T15:56:48Z

+
+public:
+  explicit CommandBuffer(GPUAPI API) : API(API) {}
+  virtual ~CommandBuffer() = default;


Classes with vtables should have an anchor to avoid object size and relocation bloat. See https://llvm.org/docs/CodingStandards.html#provide-a-virtual-method-anchor-for-classes-in-headers

bogner · 2026-04-13T16:05:48Z


+class DXCommandBuffer : public offloadtest::CommandBuffer {
+public:
+  static constexpr GPUAPI BackendAPI = GPUAPI::DirectX;


Seems like a false positive, but clang is emitting a warning for these:

E:/code/HLSLTest/lib/API/DX/Device.cpp(395,27): error: unused variable 'BackendAPI' [-Werror,-Wunused-const-variable] 395 | static constexpr GPUAPI BackendAPI = GPUAPI::DirectX; | ^~~~~~~~~~

Apparently we don't have -Werror for the pre-checkin CI jobs here, because it also showed up in those runs: https://github.com/llvm/offload-test-suite/actions/runs/24345003263/job/71083354829#step:11:4368

In any case, switching to LLVM-style RTTI will avoid the issue.

Correct, this was mostly to have backend-specific downcasting readily available when needed, for special-case tests as well as backend logic like Queue receiving generic CommandBuffer structures.

I'll look at LLVM-style RTTI as well as checking if the CI jobs can be extended with -Werror going forward as the build was always warning-free until this PR 😅

@bogner

Address post-review comments from @bogner on #1033. These felt significant enough that they weren't worth cramming into an "unrelated" PR but deserve their own clean follow-up in three individual commits. - Add virtual method anchors for all polymorphic types (`Buffer`, `CommandBuffer`, `Fence`, `ImageComparatorBase`, `ImageComparatorDiffImage`, `Texture`) per LLVM coding standards - Add file description to header comment - Switch to LLVM-style RTTI (`classof()`) instead of hand-rolled `as<T>()` + `BackendAPI`, fixing `-Wunused-const-variable` Adding `-Werror` to the build is left for a separate PR as it requires a warning audit first — the project inherits compile flags from LLVM's CMake infrastructure and backend SDK headers may introduce additional warnings. --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

MarijnS95 force-pushed the commandbuffer-abstraction branch from 378ec7a to 97e6af6 Compare March 30, 2026 13:27

llvm-beanz reviewed Mar 30, 2026

View reviewed changes

manon-traverse reviewed Mar 30, 2026

View reviewed changes

Comment thread lib/API/VK/Device.cpp Outdated

MarijnS95 force-pushed the commandbuffer-abstraction branch 2 times, most recently from 8803d26 to dadf52d Compare April 1, 2026 09:33

manon-traverse reviewed Apr 1, 2026

View reviewed changes

Comment thread lib/API/DX/Device.cpp Outdated

MarijnS95 force-pushed the commandbuffer-abstraction branch from dadf52d to 4efc0b0 Compare April 1, 2026 13:59

MarijnS95 mentioned this pull request Apr 2, 2026

Add Queue::submit() for command buffer submission #1057

Open

MarijnS95 force-pushed the commandbuffer-abstraction branch 4 times, most recently from 095fc23 to 4a67b8e Compare April 8, 2026 08:17

MarijnS95 force-pushed the commandbuffer-abstraction branch 2 times, most recently from 66bb416 to 9a1bfe5 Compare April 9, 2026 15:55

manon-traverse approved these changes Apr 9, 2026

View reviewed changes

MarijnS95 force-pushed the commandbuffer-abstraction branch 2 times, most recently from 6e5985a to 0d38e7e Compare April 13, 2026 12:45

MarijnS95 force-pushed the commandbuffer-abstraction branch from 0d38e7e to 417e140 Compare April 13, 2026 13:06

manon-traverse merged commit f1852e8 into llvm:main Apr 13, 2026
10 of 12 checks passed

MarijnS95 deleted the commandbuffer-abstraction branch April 13, 2026 14:22

bogner reviewed Apr 13, 2026

View reviewed changes

MarijnS95 mentioned this pull request Apr 14, 2026

Address post-merge review comments on CommandBuffer (#1033) #1086

Merged

		virtual llvm::Expected<std::unique_ptr<CommandBuffer>>
		createCommandBuffer() = 0;

Conversation

MarijnS95 commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bogner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MarijnS95 commented Mar 30, 2026 •

edited

Loading